Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util/log: replace gen.sh by a go template generator #57589

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

knz
Copy link
Contributor

@knz knz commented Dec 4, 2020

This prevents build problems due to folk not using a bash that's
modern enough.

Release note: None

@knz knz requested a review from itsbilal December 4, 2020 22:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20201204-log-gen branch from 5adb0d5 to 487c283 Compare December 4, 2020 22:29
@knz knz requested review from tbg and otan and removed request for tbg December 4, 2020 22:32
@knz knz force-pushed the 20201204-log-gen branch 2 times, most recently from ba84dc0 to c7451e4 Compare December 6, 2020 11:49
Makefile Outdated
docs/generated/logging.md: pkg/util/log/gen.sh pkg/util/log/logpb/log.proto
bash $< logging.md >[email protected] || { rm -f [email protected]; exit 1; }
docs/generated/logging.md: pkg/util/log/gen.go pkg/util/log/logpb/log.proto
$(GO) run $^ logging.md [email protected] || { rm -f [email protected]; exit 1; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may need $(GOMODVENDOR) flags

// If we are generating a .go file, do a pass of gofmt.
newBytes := src.Bytes()
if strings.HasSuffix(tmplName, ".go") {
newBytes, err = format.Source(newBytes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not crlfmt ;) ?

Comment string
PComment string
Name string
NAME string
Copy link
Contributor

@otan otan Dec 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not NameUpper?

I think you call also use call strings.Lower "bob" in a template, but this looks used often enough

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @otan)


Makefile, line 1548 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

may need $(GOMODVENDOR) flags

Done.


pkg/util/log/gen.go, line 69 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

why not crlfmt ;) ?

This code does not need it.

(Also crlfmt is not built to be embedded alas)


pkg/util/log/gen.go, line 97 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

why not NameUpper?

I think you call also use call strings.Lower "bob" in a template, but this looks used often enough

Yes for the same reason .NAME is shorter than .NameUpper. I cannot use .name because templates only work with exported names (:sob:)

This prevents build problems due to folk not using a `bash` that's
modern enough.

Release note: None
@knz knz force-pushed the 20201204-log-gen branch from c7451e4 to 0d98f9f Compare December 7, 2020 11:04
@knz
Copy link
Contributor Author

knz commented Dec 7, 2020

bors r=otan

@craig
Copy link
Contributor

craig bot commented Dec 7, 2020

Build succeeded:

@craig craig bot merged commit 98e724e into cockroachdb:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants